Skip to content

Allow dir-listing also when one child is blocked by access control - #5124

Merged
nickvergessen merged 2 commits into
masterfrom
allow-dirlisting-with-unreadable-items
Jun 16, 2017
Merged

Allow dir-listing also when one child is blocked by access control#5124
nickvergessen merged 2 commits into
masterfrom
allow-dirlisting-with-unreadable-items

Conversation

@nickvergessen

Copy link
Copy Markdown
Member

@mention-bot

Copy link
Copy Markdown

@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @icewind1991 and @LukasReschke to be potential reviewers.

@alterratz

Copy link
Copy Markdown

OK. 41b185b fixes the issue.
However, when an unauthorized user clicks a restricted folder the directory changes to his home directory, which not a very intuitive behavior at least. It would be nicer if this behaviour could be changed to: do not do anything at all (and eventually popup a message box "restricted....") or display an empty folder stub with some hint that the access has been denied.

@icewind1991

Copy link
Copy Markdown
Member

please dont leave commented out code lying around

@nickvergessen

Copy link
Copy Markdown
Member Author

please dont leave commented out code lying around

I will add a comment to it.
Would like to keep the code to know that we had it and what it caused...

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
nickvergessen force-pushed the allow-dirlisting-with-unreadable-items branch from 41b185b to d0c614a Compare May 26, 2017 13:54
@codecov

codecov Bot commented May 26, 2017

Copy link
Copy Markdown

Codecov Report

Merging #5124 into master will decrease coverage by 16.51%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master    #5124       +/-   ##
=============================================
- Coverage     54.14%   37.63%   -16.52%     
+ Complexity    22285    22284        -1     
=============================================
  Files          1379     1379               
  Lines         85336    85328        -8     
  Branches       1322     1322               
=============================================
- Hits          46208    32114    -14094     
- Misses        39128    53214    +14086
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Connector/Sabre/FilesPlugin.php 47.77% <ø> (-33.55%) 50 <0> (-1)
...curity/Exceptions/StrictCookieMissingException.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/Security/IdentityProof/Key.php 0% <0%> (-100%) 3% <0%> (ø)
lib/public/RichObjectStrings/Definitions.php 0% <0%> (-100%) 2% <0%> (ø)
apps/files/lib/Activity/Settings/FileFavorite.php 0% <0%> (-100%) 8% <0%> (ø)
lib/private/App/AppStore/Version/Version.php 0% <0%> (-100%) 3% <0%> (ø)
apps/files/lib/Activity/Settings/FileCreated.php 0% <0%> (-100%) 8% <0%> (ø)
...ivate/IntegrityCheck/Helpers/EnvironmentHelper.php 0% <0%> (-100%) 2% <0%> (ø)
core/Command/TwoFactorAuth/Disable.php 0% <0%> (-100%) 4% <0%> (ø)
...pFramework/Db/MultipleObjectsReturnedException.php 0% <0%> (-100%) 1% <0%> (ø)
... and 474 more

@JiriMON JiriMON left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alterratz

alterratz commented May 28, 2017

Copy link
Copy Markdown

The pull request fixes the issue but it open an other can of worms:
file access control can be removed if user has access to parent folder #66
(which is for me a grave issue since it effectively disables access control based on the FAC application)
I suppose this is the reason why the lines commented out in the pull request were there in the first place.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen

Copy link
Copy Markdown
Member Author

@MorrisJobke @icewind1991 please review as this needs backporting for 12.0.1

@icewind1991

Copy link
Copy Markdown
Member

Instead of not throwing the exception it seems better to me to handle the exception in folder listing and hide the problematic entry

@nickvergessen

Copy link
Copy Markdown
Member Author

That's bad, as it would cause sync clients to delete the file?

@nickvergessen

Copy link
Copy Markdown
Member Author

Lets just revert the change from vincent which caused this by accident. Our codebase worked fine without it

@nickvergessen

Copy link
Copy Markdown
Member Author

there are 2 +1 and robin +1ed the comment, so merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants